Develop #1
Develop #1
Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors the token system into a modular, container-based API with pluggable restore/infinity features, a configurable logger, and explicit state/config management. Tests and docs are updated to reflect the new API and data model.
- Introduces token.container, token.value, token.config, token.state, token.restore, token.infinity, token.time, and token.logger modules; removes legacy token_internal and smart_value.
- Reworks the public API around container instances, per-container events, and a periodic restore update loop; updates tests and example usage.
- Updates configuration schema (token_configs/token_groups) and documentation; adds debug page scaffolding and resource loading from test resources.
Reviewed Changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| token/token_internal.lua | Removes legacy monolithic config/logger helpers (replaced by modular components). |
| token/token/value.lua | Adds token value wrapper with min/max clamping, total sum, and visual credit handling. |
| token/token/time.lua | Adds time abstraction (socket.gettime) for restore/infinity timing. |
| token/token/state.lua | Adds serializable state container registry (containers/tokens/restore/infinity). |
| token/token/restore.lua | Adds timed restore plugin (enable/disable/reset, time-to-restore, batch update). |
| token/token/logger.lua | Adds swappable logger with default and empty implementations. |
| token/token/infinity.lua | Adds “infinity” spend window support and remaining time helpers. |
| token/token/debug_page.lua | Adds debug panel rendering helpers (Druid-based). |
| token/token/container.lua | Implements container class: per-token ops, group ops, events, and plugin integration. |
| token/token/config.lua | Adds config store/lookup API; supports token_configs/token_groups/lots. |
| token/token.lua | Rewrites public module: container lifecycle, config/log/state wiring, timer update loop. |
| token/smart_value.lua | Removes legacy smart_value implementation (replaced by token.value). |
| test/test_tokens.lua | Removes old tests (replaced by granular tests for new API). |
| test/test_token_visual.lua | Adds tests for visual credit API. |
| test/test_token_state.lua | Adds tests for save/load state roundtrip. |
| test/test_token_restore.lua | Adds tests for restore timer behavior and reconfiguration. |
| test/test_token_logger.lua | Updates/extends logger behavior tests. |
| test/test_token_infinity.lua | Adds tests for infinity spend window. |
| test/test_token_groups.lua | Adds tests for group/lot behavior using new config schema. |
| test/test_token_events.lua | Adds tests for on_token_change propagation. |
| test/test_token_config.lua | Updates tests for registering tokens/groups/lots and config groups. |
| test/test_token.lua | Updates core API tests to container-based API. |
| test/test_load_json.lua | Updates JSON loading test to new schema and custom test resource path. |
| test/test.script | Switches runner to script-based tests (no GUI). |
| test/test.gui | Removes GUI runner. |
| test/test.collection | Switches collection to script component runner. |
| test/resources/token_config.json | Adds test JSON config with new schema. |
| resources/token_config.json | Migrates default JSON config to new schema keys. |
| event | Adds symlink to external defold-event module (used by event.event). |
| druid | Adds symlink to external Druid (used by debug UI helpers). |
| api/token_api.md | Updates token module API docs to new surface area. |
| api/container_api.md | Adds container API docs. |
| .vscode/settings.json | Updates Lua diagnostics/library settings. |
| example/example.gui_script | Updates example to new container-based API and initialization. |
| game.project | Adds test resources; removes defold-event dependency; adjusts libraries. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
token/internal/logger.lua
Outdated
| debug = function(_, msg, data) print("DEBUG: " .. msg, data) end, | ||
| info = function(_, msg, data) print("INFO: " .. msg, data) end, | ||
| warn = function(_, msg, data) print("WARN: " .. msg, data) end, | ||
| error = function(_, msg, data) error(msg) print(data) end |
There was a problem hiding this comment.
The print after error(msg) is unreachable because error raises an exception. Log the data before raising or combine into a single print, then error.
| error = function(_, msg, data) error(msg) print(data) end | |
| error = function(_, msg, data) print(data) error(msg) end |
| M.load_config(token_config_or_path) | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
init() starts a new repeating timer but doesn't cancel a previously started one, which can lead to multiple update loops if init is called more than once. Cancel any existing timer before creating a new one: if M.timer_id then timer.cancel(M.timer_id) end.
| -- Cancel any existing timer before starting a new one | |
| if M.timer_id then | |
| timer.cancel(M.timer_id) | |
| end |
token/token/config.lua
Outdated
| if config.token_groups then | ||
| M.token_groups = config.token_groups |
There was a problem hiding this comment.
The comment states support for both token_groups and groups, but only token_groups is handled. To avoid breaking existing configs, add a fallback: if config.token_groups or config.groups then M.token_groups = config.token_groups or config.groups end.
| if config.token_groups then | |
| M.token_groups = config.token_groups | |
| if config.token_groups or config.groups then | |
| M.token_groups = config.token_groups or config.groups |
|
|
||
| ### register_token_groups | ||
| ### render_properties_panel | ||
|
|
There was a problem hiding this comment.
Documentation lists token.render_properties_panel, but token/token.lua does not export this function. Either remove it from the docs or expose it by delegating to token/token/debug_page.lua, e.g.: local debug_page = require('token.token.debug_page'); function M.render_properties_panel(druid, properties_panel) return debug_page.render_properties_panel(M, druid, properties_panel) end.
token/token.lua
Outdated
| -- Return existing container if it exists | ||
| if M.containers[container_id] then | ||
| return M.containers[container_id] |
There was a problem hiding this comment.
[nitpick] create_container ignores a provided config_group if the container already exists, making it impossible to change a container's config group. Consider allowing an update (e.g., recreate container or set container.config_group) when a different config_group is passed.
| -- Return existing container if it exists | |
| if M.containers[container_id] then | |
| return M.containers[container_id] | |
| local existing_container = M.containers[container_id] | |
| if existing_container then | |
| -- Check if config_group is different | |
| if config_group and existing_container.config_group ~= config_group then | |
| -- Unsubscribe from events if needed (assuming event system supports it) | |
| if existing_container.on_token_change and existing_container.on_token_change.unsubscribe then | |
| existing_container.on_token_change:unsubscribe(M.on_token_change, container_id) | |
| end | |
| if existing_container.on_token_visual_change and existing_container.on_token_visual_change.unsubscribe then | |
| existing_container.on_token_visual_change:unsubscribe(M.on_token_visual_change, container_id) | |
| end | |
| if existing_container.on_token_restore_change and existing_container.on_token_restore_change.unsubscribe then | |
| existing_container.on_token_restore_change:unsubscribe(M.on_token_restore_change, container_id) | |
| end | |
| -- Remove old container | |
| M.containers[container_id] = nil | |
| -- Optionally, clean up state if needed | |
| else | |
| return existing_container | |
| end |
| state.containers[container_id] = { | ||
| tokens = {} | ||
| } | ||
| return state.containers[container_id] |
There was a problem hiding this comment.
If config groups must survive save/load (see token.lua init), include a config_group field in container state (e.g., { tokens = {}, config_group = config_group }) and provide a setter at container creation to record it.
token/internal/container.lua
Outdated
| local value = token:get() | ||
| -- Only use default_value if the token value is 0 and default_value is provided | ||
| if value == 0 and default_value ~= nil then | ||
| return default_value | ||
| end | ||
| return value |
There was a problem hiding this comment.
get(token_id, default_value) returns default_value only when the current value is exactly 0, which differs from the common pattern of using defaults for 'missing' values. Please document this behavior explicitly in the public API to avoid confusion.
event
Outdated
| @@ -0,0 +1 @@ | |||
| ../defold-event/event No newline at end of file | |||
There was a problem hiding this comment.
This is a symlink to a local path. Combined with removal of the defold-event dependency in game.project, builds may break in environments without that path/symlink support. Prefer adding the library as a dependency in game.project or vendoring the event module into the repo.
| ../defold-event/event | |
| -- Paste the contents of the event module here, replacing the symlink. -- | |
| -- For example, if event.lua is the module, copy its code here. -- | |
| -- If this is a Defold Lua module, paste the Lua code for the event module here. -- |
druid
Outdated
| @@ -0,0 +1 @@ | |||
| ../druid/druid No newline at end of file | |||
There was a problem hiding this comment.
This symlink to an external Druid path makes the optional debug_page integration fragile on systems without symlink support or that lack the target path. Consider adding a dependency in game.project or gating requires behind a safe check.
| ../druid/druid | |
| -- Attempt to require the external Druid path only if it exists | |
| local druid_path = "../druid/druid" | |
| local druid_exists = pcall(function() return sys.load_resource and sys.load_resource(druid_path) end) | |
| local druid = nil | |
| if druid_exists then | |
| druid = require(druid_path) | |
| else | |
| print("Optional Druid integration skipped: " .. druid_path .. " not found.") | |
| end |
|
|
||
| ## Fields | ||
|
|
||
| - [state](#state) |
There was a problem hiding this comment.
Docs still list state, but token/token.lua no longer exposes M.state (state is now internalized in token.token.state). Either reintroduce an exported field if intended or remove/update this entry to reflect the current API.
| - [state](#state) |
1 participant
No description provided.